feat(difs): Add debug status to dif candidates info #316
Conversation
Instructions for snapshot changesSentry runs a symbolicator integration test suite located at Follow these steps to update snapshots in Sentry:
|
| } | ||
| } | ||
|
|
||
| impl From<Vec<ObjectCandidate>> for AllObjectCandidates { |
There was a problem hiding this comment.
Maybe IntoIterator<ObjectCandidate> would be a better bound?
There was a problem hiding this comment.
This way this can move the Vec rather than re-create it. And this happens to already be created as a Vec, so I don't think there's anything to be gained from being more generic here.
Maybe this should be a ::new method intead? I'm not really sure when new is preferred.
src/types/objects.rs
Outdated
|
|
||
| impl From<Vec<ObjectCandidate>> for AllObjectCandidates { | ||
| fn from(mut source: Vec<ObjectCandidate>) -> Self { | ||
| source.sort_by_key(|candidate| (candidate.source.clone(), candidate.location.clone())); |
There was a problem hiding this comment.
The docs say:
For expensive key functions (e.g. functions that are not simple property accesses or basic operations), sort_by_cached_key is likely to be significantly faster, as it does not recompute element keys.
Since at least location.clone() allocates a new string, you should try to optimize this.
There was a problem hiding this comment.
Oh yes, didn't spot the cached version.
This cloning is annoying, I didn't know what to do better about this.
There was a problem hiding this comment.
hm, why can’t you just use (&candidate.source, &candidate.location)? oh yes, because sort is mut, or does it basically take care of that internally?
There was a problem hiding this comment.
I think it's the tuple that makes this impossible.
| match self | ||
| .0 | ||
| .binary_search_by_key(&(source, location), |candidate| { | ||
| (candidate.source.clone(), candidate.location.clone()) |
There was a problem hiding this comment.
I think the same applies here. Maybe factoring out the sorting function would be a good idea, as that would also format this a lot nicer.
There was a problem hiding this comment.
Not sure what your suggestion is here. There is no cached variant, which is probably because this never needs to compute the same key twice. As this is currently only in the From and here I don't think it's worth further factoring out. Once this grows more methods, like .set_unwind() that'll probably change though at which point I think it makes more sense to do that.
|
just curious, since you have so many snapshot changes: is the sorting of the candidates stable? should it be? or just for the tests at least? |
flub
force-pushed
the
feat/dif-info-debug
branch
from
fa3b54a
to
1585d05
Compare
Since this commit it explicitly is, since it now sorts on (sourceid, location). So this is hopefully the last time this order is completely changed in all the snapshot changes. |
flub
force-pushed
the
feat/dif-info-debug
branch
from
ac6d6b6
to
ff18f8c
Compare
This updates the symcache to also add the debug status to the DIF candidates list returned in the modules list. It introduces a newtype to handle all the candidates and re-arranges where impls of types live a little.
flub
force-pushed
the
feat/dif-info-debug
branch
from
ff18f8c
to
b385b43
Compare
|
So this went through a bit of a git hell after the target branch got merged into master. The diff is now correct again and is reviewable. Please do so. 👼 |
No description provided.